-
Notifications
You must be signed in to change notification settings - Fork 66
FXC-3517 add impedance calculations to mode solver #2837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
28 files reviewed, 2 comments
ceccb9e
to
18ced37
Compare
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/geometry/utils.py
tidy3d/components/microwave/base.py
tidy3d/components/microwave/path_integrals/factory.py
tidy3d/components/microwave/path_integrals/integrals/base.py
tidy3d/components/microwave/path_integrals/integrals/current.py
tidy3d/components/mode/mode_solver.py
tidy3d/plugins/microwave/auto_path_integrals.py
tidy3d/plugins/microwave/custom_path_integrals.py
tidy3d/plugins/microwave/impedance_calculator.py
tidy3d/plugins/microwave/path_integrals.py
|
6996084
to
290484a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most went through code structures, and the structure looks in good shape. Some minor comments:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very impressive work, finished the first pass and left some comments. Additionally, it seems like many new classes AxisAlignedPathIntegralSpec
, CustomPathIntegral2DSpec
, CurrentIntegralAxisAlignedSpec
, CustomCurrentIntegral2DSpec
, CompositeCurrentIntegralSpec
, CustomImpedanceSpec
, etc are missing examples in docstrings
7ad7a12
to
2727e2f
Compare
917c3ad
to
d23f936
Compare
Thanks @dbochkov-flexcompute for finding those docstring mistakes. It was a little bit harder than expected to get doc tests running correctly because of the log warning we have for rf and microwave components. I got it working by introducing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good @dmarek-flex ! Just a few questions on very few API design considerations
837a080
to
c685b6b
Compare
bee9d40
to
008ab9f
Compare
Alright lets have another go @greptileai. Also since @daquinteroflex is away, can @tylerflex or @yaugenst-flex take a look at this refactoring of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
tidy3d/plugins/microwave/auto_path_integrals.py
, line 32-34 (link)logic: Parameter descriptions don't match parameter names -
lumped_element
andgrid
descriptions are incorrectContext Used: Rule from
dashboard
- Keep docstrings and comments synchronized with code changes to ensure they are always accurate and n... (source)
63 files reviewed, 9 comments
b2458d6
to
40bcc17
Compare
We should figure out a plan regarding the RF/Tidy3D separation and organization long term. @weiliangjin2021 just for a sense, here are some related discussions #2837 (comment) My feeling is that the best long term solution is a complete separation of RF from the main tidy3d components. Meaning perhaps a But for this to be a reality, we would need to (at least):
Could you guys think about this a bit in terms of requirements from your side and any preferences? @weiliangjin2021 let's discuss together with yannick and Lucas at the tech council meeting monday to get a general feeling because the same issues arise in photonforge. |
I just started looking at this: One idea occurred to me, which is that if we eventually do want to import While underlying things would still be all mixed without any changes, this would at least give us an ability to start crafting the user-facing side of the package. note sure if worth doing here since this is a needed feature, but wanted to make a note. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we've decided to refactor this in a separate PR into tidy3d.rf, for now feel free to merge but won't be like this for rc3
Will do another review pass tomorrow my morning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the few minor comments
27d9f00
to
9aa1b9b
Compare
Just made some final tests with the deployed solver and everything looks good. I decided to submit the docs changes in a different PR so that we can take our time with that #2897 @dbochkov-flexcompute Let me know if you approve of the changes regarding the license warning. |
9aa1b9b
to
7a5edfc
Compare
add a ImpedanceSpec for controlling how characteristic impedance is calculated from modes add MicrowaveModeMonitor and MicrowaveModeSolverMonitor that accepts the new MicrowaveModeSpec BREAKING CHANGE: changed path integral class names in plugins/microwave
7a5edfc
to
c8f2694
Compare
add a ImpedanceSpec for controlling how characteristic impedance is calculated from modes
backend PR: https://github.com/flexcompute/compute/pull/2497
- [ ] Finalize docs including the new Microwave components somewhere (monitors/mode_specs/path_specs)Now tracking here
Greptile Overview
Updated On: 2025-10-07 20:11:19 UTC
Summary
This PR introduces a comprehensive impedance calculation system for microwave mode solvers in Tidy3D. The implementation adds the ability to calculate characteristic impedance from electromagnetic modes, which is essential for microwave and RF transmission line analysis.Key architectural changes:
ImpedanceSpecTypes
union withAutoImpedanceSpec
for automatic impedance calculation andCustomImpedanceSpec
for user-defined voltage/current path integralsMicrowaveModeSpec
extending the base mode specification with impedance calculation capabilities for each modeModeSpec
,ModeSolver
, and related classes to support microwave specifications while maintaining backward compatibilityMicrowaveModeMonitor
andMicrowaveModeSolverMonitor
for microwave-specific data collectionMicrowaveModeData
andMicrowaveModeSolverData
classes that include transmission line characteristics (impedance, voltage/current coefficients)Implementation highlights:
ModePlaneAnalyzer
MicrowaveBaseModel
base class for consistent RF licensing and functionalityVoltageIntegralAxisAligned
→AxisAlignedVoltageIntegral
)The system integrates seamlessly with existing Tidy3D workflows - users can continue using standard mode specifications for basic electromagnetic analysis while accessing advanced impedance calculations through the new microwave specifications. The architecture properly separates automatic impedance calculation for common cases from custom path integral specifications for complex geometries.
PR Description Notes:
Important Files Changed
Changed Files
tidy3d/components/microwave/path_integrals/impedance_spec.py
tidy3d/components/microwave/microwave_mode_spec.py
tidy3d/plugins/microwave/impedance_calculator.py
tidy3d/components/mode_spec.py
tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py
tidy3d/components/mode/mode_solver.py
tidy3d/components/microwave/path_integrals/base_spec.py
tidy3d/components/microwave/path_integrals/voltage_spec.py
tidy3d/components/microwave/path_integrals/current_spec.py
tidy3d/components/microwave/monitor.py
tidy3d/components/microwave/data/monitor_data.py
tidy3d/components/microwave/base.py
tidy3d/components/simulation.py
tidy3d/components/geometry/utils.py
tidy3d/plugins/microwave/path_integrals.py
tidy3d/plugins/microwave/custom_path_integrals.py
docs/api/microwave/microwave_migration.rst
tidy3d/__init__.py
CHANGELOG.md
Confidence score: 3/5
tidy3d/components/mode_spec.py
for the missing closing quote in the validation error messageContext used:
Rule from
dashboard
- Do not use markdown formatting in exception or warning messages; use single quotes to highlight vari... (source)Rule from
dashboard
- Update the CHANGELOG.md file when making changes that affect user-facing functionality or fix bugs. (source)